Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warnings cleanup #5964

Merged
merged 11 commits into from Jun 27, 2019
Merged

Warnings cleanup #5964

merged 11 commits into from Jun 27, 2019

Conversation

JohnHolmesII
Copy link
Contributor

Some very important things to note:

  • I used travis as a reference for cleaning the warnings. I did not pay any mind to other compilers. There is work still to do. MSVC seems a lot more verbose.
  • I tried my best to make commits that had no effect on the emu itself, for better or worse. The "solutions" I have commited may or may not be ideal.
  • I have also tried to make every commit silence a single class of warnings, so that this can be cherry picked.

Any feedback from all contributors is encouraged.

rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/RSXThread.cpp Outdated Show resolved Hide resolved
@@ -517,6 +518,7 @@ namespace vk
barrier.srcAccessMask = VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_INPUT_ATTACHMENT_READ_BIT;
src_stage = VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT;
break;
default: break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default should throw "Attempted to transition from invalid layout"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this change causes Vulkan to crash the emulator in all games, as discovered by our favorite Finn Jotain. Falling through allows normal behavior.

rpcs3/Emu/RSX/VK/VKHelpers.h Outdated Show resolved Hide resolved
rpcs3/Emu/RSX/VK/VKHelpers.h Outdated Show resolved Hide resolved
@JohnHolmesII
Copy link
Contributor Author

I've got a few commits elsewhere cleaning up the third party warnings, but I'm still working on trying to get the include dirs passed to clang cleaned up. That'll clean up another few hundred errors.

If anyone with knowledge about some of the remaining rpcs3 warnings (@elad335 @Nekotekina maybe?) and think of a way to resolve them, I can put those changes here too. The problem is that those instructions are wrapped in macros that disable them for missing instruction sets. However the macros are just left blank, generating a warning. For example, after the preprocessor is run, we get code like the following
const auto sa = (A, index);

which is just weird

@JohnHolmesII JohnHolmesII changed the title Warnings cleanup [WIP] Warnings cleanup May 15, 2019
@JohnHolmesII
Copy link
Contributor Author

I've gone ahead and implemented the changes to silence 3rd party code. I put in some PoC workarounds for the macro issues; these changes may or may not be okay.

Currently, the clang travis build produces no warnings of any kind. The log has gone from 8665 lines to 1220.

I still have more plans. Neko recently turned on extra warnings (-Wall) for gcc that was not turned on for clang. I plan to unify the flags between the 2 compilers, since they are mostly compatible. I will try to get as close to exact -Wall compliance as I can in future commits. Perhaps even -Wextra if enough gets done. Hopefully we can maintain this into the future.

@MSuih
Copy link
Member

MSuih commented May 15, 2019

The 3rdparty warning silence doesn't seem to work on MSVC (using CMake). Not only does it let errors through but it now also spams cl : Command line warning D9025 : overriding '/w' with '/W4' for every single compiled LLVM file and similar erros for some other 3rdparty stuff. My setup is a bit unusual though, I don't expect full support for it.

I can upload the whole compile output somewhere if you're interested in wading through it.

@MSuih
Copy link
Member

MSuih commented May 15, 2019

Also, did anyone test running games on this? Double clicking anything on the game list causes a fatal error to pop up. Happens on both appveyor builds and the stuff I compiled myself.
image

@JohnHolmesII
Copy link
Contributor Author

Indeed, I did not add all the warning wrappers to msvc, just the one. msvc is at the bottom of my priority list right now, since I can barely read the output and my branch is on linux.
s crashing I think it may be inn regards to the shader strings being modified (only thing i can think of. I wont be able to update this pr for a few days, but Ill take a look at it when I get the chance.
In regards to the game

@JohnHolmesII
Copy link
Contributor Author

I've pushed an update with quite a bit of changes to what's going on. When adding -Wall to the compilers, many more warnings were exposed. It was both too many for me to handle, and also well outside my knowledge to fix. So while refactoring the cmake setup to be (hopefully) more clear than it was before, I decided to simply enumerate all the warnings as silenced, rather than not use -Wall.

This may seem odd (and that list is disturbingly long), but it provides a number of advantages. For one, the sight of it may encourage devs to begin working on them. For another, the log output is much cleaner, meaning that new PRs will have new warnings immediately highlighted for all to see, preventing issues from creeping in.

It also provides a neat little way for contributors to help the issue, as this is larger than one dev can handle. Simply pop a warning off the silenced list, repair the necessary code until it is fixed, then open a PR. Recently several PRs were introduced that had small warning-fix scope, and hopefully we can get more of those.

I still have plans here.

  1. GCC has an old issue rearing its head, Linux: not used because `__STDC_CONSTANT_MACROS' is defined [-Winvalid-pch] #1818. It seems to have come back either in recent GCCs, or because I'm an idiot, or both. Before there was a warning silence in place (-Wno-invalid-pch), but readding this doesn't seem to help, likely because it is simply in the wrong place. It might be easier to fix than it looks, so I will look into that first.
  2. MSVC has not been worked on at all, and I will have to do that. I have a wild fantasy that we might switch the windows build to clang and I won't have to do any work, but I cannot rely on that, given the "inertia" of the project.
  3. Find a way to slap on -Werror. This will be useful as new PRs will clearly have warnings marked by CI, and it can't slip past reviewers who might be focused elsewhere.

And of course, I will be addressing the other review comments.

@JohnHolmesII
Copy link
Contributor Author

Also, my changes obviate #4989, so when this is done that can be closed.

@AniLeo
Copy link
Member

AniLeo commented Jun 24, 2019

Please rebase and resolve current reviews, other warnings can be cleaned up in a follow-up PR
This needs to be merged before hcorion's 16.04 PR

@JohnHolmesII
Copy link
Contributor Author

Squashed & rebased. Clang is clean, but GCC has problems with PCH still. That may be why GCC takes so much longer to build. I tried to look into it, but I get a different error than what is seen on travis. Someone with knowledge of cotire needs to take a look. Also, GCC is giving "notes", which seemingly can't be turned off, as they aren't even warnings. The changes required to fix the note spam is best handled by someone with knowledge of the objects.

Anyway, I think this is ready for merge, barring last minute review. The work on MSVC can be done separately.

@JohnHolmesII JohnHolmesII changed the title [WIP] Warnings cleanup Warnings cleanup Jun 25, 2019
@AniLeo AniLeo requested a review from Nekotekina June 25, 2019 14:12
@AniLeo
Copy link
Member

AniLeo commented Jun 25, 2019

Cancelled current build due to big queue, rebase after latest Nekotekina hotfix is merged for a new build, saves compiling time

@JohnHolmesII
Copy link
Contributor Author

Travis seems quiet. Is it ok to push?

Add default cases.
Move default breaks to newline
Add proper handling in some instances.
Add missing enums to switches
Explicitly mark loop types (per review)
-Indentation warnings
-prevent shift overflow
-This was declared extern in all contexts. Remove this for initialization
-Fix main return types. OH CANADA!
-Silence extraneos 'unused expression' warning
-Force use return value (warning)
-Remove tautological compare copy-pasta (char always < 256)
@Nekotekina Nekotekina merged commit 232a35b into RPCS3:master Jun 27, 2019
@MSuih MSuih mentioned this pull request Jun 28, 2019
@dio-gh dio-gh mentioned this pull request Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants